feat: implement asset callbacks#2571
Conversation
cdb8e5f to
7588e08
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! Not a full review yet, but I left some comments inline.
| "auth_procedure": 62667, | ||
| "after_tx_cycles_obtained": 574 | ||
| "total": 71195, | ||
| "auth_procedure": 69694, |
There was a problem hiding this comment.
Auth procedure cycle count went up quite a bit - but I'm guessing this may be because of some prior changes rather than because of this PR, right?
There was a problem hiding this comment.
Checked a few key PRs:
before auth component consolidation: 9cfb4f2a
"auth_procedure": 62641
before asset refactor: a09027b6
"auth_procedure": 63170
before VM migration: e8570061
"auth_procedure": 63169
after VM migration: 0897aae6
"auth_procedure": 69686
So auth component consolidation added ~500 cycles but the VM migration was the main reason for the large increase in cycles.
There was a problem hiding this comment.
Since most of the auth procedure is Falcon signature verification - I wonder if most of the delta is because Falcon signature verification got a bit less efficient.
@Al-Kindi-0 - could you check how many cycles Falcon signature takes now? (maybe some stack re-orientation work made it less efficient?)
crates/miden-protocol/asm/kernels/transaction/lib/callbacks.masm
Outdated
Show resolved
Hide resolved
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one small comment inline.
mmagician
left a comment
There was a problem hiding this comment.
Do not allow callbacks to mint anything; keep them read-only in general. For now this is the case and that may be the best option until we have thought about this more thoroughly.
Is this true that right now this is the case? Looking at the procedure signature:
#! Inputs: [ASSET_KEY, ASSET_VALUE]
#! Outputs: [PROCESSED_ASSET_VALUE]
#!
#! Where:
#! - ASSET_KEY is the vault key of the asset being added.
#! - ASSET_VALUE is the value of the asset being added.
#! - PROCESSED_ASSET_VALUE is the asset value returned by the callback, or the original
#! ASSET_VALUE if callbacks are disabled.
pub proc on_before_asset_added_to_account
It looks like PROCESSED_ASSET_VALUE could mint new tokens
The signature is setup so that we could allow this in the future. At the moment, foreign accounts have read-only access to the transaction, so they effectively cannot mint/burn tokens. If/Once we allow that, the callback would be able to mint/burn tokens as well. |
Implements the
on_before_asset_added_to_accountcallback in the tx kernel.The PR is deliberately "minimal" and does not:
on_before_asset_added_to_notecallback.Both of these will be done in a separate PR based on feedback to this PR.
Design Decisions
Allow missing callback slots
Since the callback flag is part of the asset key, the callback flag must be set whenever an asset is created, e.g. in
miden::protocol::asset::create_fungible_asset. So, this and similar procedures now takeenable_callbacksas an input.To avoid parameterizing implementations like
miden::standards::faucets::distributeover this flag, a helper is added that checks whether the active account defines any callbacks:miden::protocol::active_account::has_callbacks. This procedure is called in distribute and passes the returned flag on to the to-be-minted asset.Decisions
has_callbacksreturns 1 if the slot exists and contains a non-empty word, 0 otherwise. This is to avoid having to add the callback slots to all faucet accounts (and in the future all accounts).miden::protocol.The 2nd decision was made by considering the following (post-mainnet) scenario:
has_callbacksis a library-only implementation.BasicFungibleFaucetuses the flag returned by this procedure to create it assets.has_callbackscannot be updated, it is deprecated and ahas_callbacks_v2is added that implements the existence check for the new callback.BasicFungibleFaucetcode still uses the MAST root of the originalhas_callbacksprocedure and now consequently determines that it does not have callbacks, even though the new one is actually used.When the
has_callbacksproc is implemented in the kernel, due to dynamic invocation, it can be upgraded to take into account newly added callbacks without breaking MAST roots.In theory I would prefer the library-only implementation, but I'm not sure how to properly address such issues otherwise.
Worth noting: A similar problem exists for
miden::standards::auth::signature::verify_signature_by_scheme- adding new schemes in the future will change the MAST root and we can't apply the kernel procedure workaround there.Multiple Procedures Per Callback
This PR assumes the need to assign multiple procedures to the same callback is unlikely to come up. If it would come up anyway, account components would have to manually merge multiple procedures into a single one and then provide that one as the callback procedure.
The approach here is that account components that implement callbacks need to add the callback storage slot themselves, with the help of
AssetCallbacks.Callback Inputs
on_before_asset_added_to_accountdoes not pass the native account ID to the callback to keep the signature simpler. It is easy to retrieve the ID withnative_account::get_id. The main idea is:Applying this consistency rule from the account to the note callback would mean that the note callback would become a bit messy if it were to provide
[note_idx, native_account_id_suffix, native_account_id_prefix, ASSET_KEY, ASSET_VALUE], and so it makes sense to me to not pass the native ID to any of them.Callback Security
I'm also wondering about the security of callback procedures in an account's public interface. If the callback is a public part of the interface, it can be called by anyone. But if it turns out that callbacks will be able to mint assets, then this could probably be exploited:
ASSET_VALUEof the faucet's asset and invoke the callback from a tx script.PROCESSED_ASSET_VALUE.PROCESSED_ASSET_VALUE - ASSET_VALUEis a valid asset that can be sent to some other account.Mitigations
authenticate_account_origin, if the called procedure is a callback, check whether the kernel is in a "callback context". Not yet sure if this protects against all possible ways this could be called.FungibleAssetDelta
The
FungibleAssetDeltaneeded an update since the (Rust) delta only included the faucet ID, but the callback flag also needs to be included to differentiate assets with and without callbacks. This means including the faucet ID and the asset metadata (= currently callback flag).This will become much nicer when we generalize the asset delta, so I did not spend time on optimizing the code in that type.